Random: add DRBG reseed support#130
Conversation
dgarske
left a comment
dgarske
left a comment
There was a problem hiding this comment.
Skoll Code Review
Scan type: reviewOverall recommendation: COMMENT
Findings: 4 total — 4 posted, 0 skipped
4 finding(s) posted as inline comments (see file-level comments below)
Posted findings
- [Medium] test_reseed_empty asserts nothing and silently passes on either outcome —
tests/test_random.py:61-66 - [Low] Non-ASCII em-dash in test comment violates ASCII-only convention —
tests/test_random.py:66 - [Low] test_reseed only checks output length, not that reseed had any effect —
tests/test_random.py:55-58 - [Low] Per-method feature gate inside class body differs from repo's module-level gating pattern —
wolfcrypt/random.py:81-89
Review generated by Skoll
dgarske
left a comment
dgarske
left a comment
There was a problem hiding this comment.
Skoll Multi-Scan Review
Modes: review + review-securityOverall recommendation: COMMENT
Findings: 3 total — 3 posted, 0 skipped
3 finding(s) posted as inline comments (see file-level comments below)
Posted findings
- [Medium] [review] reseed error branch missing # pragma: no cover used by every other error path —
wolfcrypt/random.py:88 - [Low] [review] Trailing whitespace on blank line inside test_reseed_multiple —
tests/test_random.py:79 - [Low] [review] Seed bytes use % 255, likely intended % 256 —
tests/test_random.py:61,77
Review generated by Skoll
- add pragma no cover - format using ruff - fix module factor in test_random
dgarske
left a comment
dgarske
left a comment
There was a problem hiding this comment.
Skoll Code Review
Scan type: reviewOverall recommendation: COMMENT
Findings: 3 total — 3 posted, 0 skipped
3 finding(s) posted as inline comments (see file-level comments below)
Posted findings
- [Medium] HASHDRBG feature not detected on Windows non-FIPS build; reseed() silently unavailable —
scripts/build_ffi.py:381 - [Low] test_reseed_multiple is non-deterministic —
tests/test_random.py:72-85 - [Low] reseed() accepts bytes only with no input normalization —
wolfcrypt/random.py:82-89
Review generated by Skoll
| features["ML_DSA"] = 1 if '#define HAVE_DILITHIUM' in defines else 0 | ||
| features["ML_KEM"] = 1 if '#define WOLFSSL_HAVE_MLKEM' in defines else 0 | ||
| features["HKDF"] = 1 if "#define HAVE_HKDF" in defines else 0 | ||
| features["HASHDRBG"] = 1 if "#define HAVE_HASHDRBG" in defines else 0 |
There was a problem hiding this comment.
🟠 [Medium] HASHDRBG feature not detected on Windows non-FIPS build; reseed() silently unavailable
The new detection features["HASHDRBG"] = 1 if "#define HAVE_HASHDRBG" in defines else 0 does a literal, exact-line match against the defines pulled from options.h/user_settings.h. On Linux/macOS the autotools build emits #define HAVE_HASHDRBG into the generated options.h (verified in lib/wolfssl/.../include/wolfssl/options.h), so detection works. On Windows non-FIPS the build copies windows/non_fips/user_settings.h as the sole defines source, and that file does NOT contain #define HAVE_HASHDRBG — Hash-DRBG is left enabled implicitly via wolfSSL's default settings.h (no WC_NO_HASHDRBG). As a result, features["HASHDRBG"] evaluates to 0 on Windows non-FIPS even though wc_RNG_DRBG_Reseed is actually compiled into the library. Consequently HASHDRBG_ENABLED is 0, wc_RNG_DRBG_Reseed is omitted from the cdef, and Random.reseed() is never defined on the default Windows build — the very feature this PR adds ships as unavailable there. Note windows/fips_ready/user_settings.h:17 already defines HAVE_HASHDRBG, and windows/non_fips/user_settings.h:23 already lists HAVE_HKDF for exactly this reason, so the omission looks accidental.
Fix: Add #define HAVE_HASHDRBG to windows/non_fips/user_settings.h (mirroring the existing HAVE_HKDF define and the fips_ready variant) so the new detection fires and reseed() is exposed on the default Windows build. Alternatively, document that reseed is Linux/macOS-only for now.
|
|
||
|
|
||
| @pytest.mark.skipif(not _lib.HASHDRBG_ENABLED, reason="Reseeding only available with hash-DRBG") | ||
| def test_reseed_multiple(rng): |
There was a problem hiding this comment.
🔵 [Low] test_reseed_multiple is non-deterministic
The test derives both the per-iteration seed size and the final byte count from live RNG output (seed_size = ord(rng.byte()), num_bytes = ord(rng.byte())). This makes the test path non-deterministic: a failure would not reproduce with a fixed input, and coverage varies run to run. Values are bounded to 0-255 so there is no resource risk, but reproducibility suffers. The success-path coverage itself is otherwise good (test_reseed_sizes already exercises 0/1/32/1000).
Fix: Use fixed, explicit seed sizes and byte counts so the test is deterministic and reproducible; keep the loop to prove consecutive reseeds work.
| return _ffi.buffer(result, length)[:] | ||
|
|
||
| if _lib.HASHDRBG_ENABLED: | ||
| def reseed(self, seed: __builtins__.bytes) -> None: |
There was a problem hiding this comment.
🔵 [Low] reseed() accepts bytes only with no input normalization
reseed passes seed straight to the CFFI call. If a caller passes a str instead of bytes, CFFI raises an opaque type error rather than a clear message. Other modules (e.g. hkdf.py) normalize inputs via t2b(). This is consistent with Random.__init__ (which also takes nonce as raw bytes without t2b), so within random.py the convention is followed; flagging only for awareness since the type hint (__builtins__.bytes) is not enforced at runtime.
Fix: Optional: either leave as-is to match Random.init's bytes-only convention, or run seed through t2b() for consistency with the rest of the bindings and friendlier errors on str input.
3 participants
Only available when Hash-DRBG is enabled, which is the default.